Skip to content

Add checked_apply helper for forwarding configclass fields onto upstream dataclasses#5365

Merged
kellyguo11 merged 5 commits intoisaac-sim:developfrom
hujc7:jichuanh/upstream-set-helper
Apr 26, 2026
Merged

Add checked_apply helper for forwarding configclass fields onto upstream dataclasses#5365
kellyguo11 merged 5 commits intoisaac-sim:developfrom
hujc7:jichuanh/upstream-set-helper

Conversation

@hujc7
Copy link
Copy Markdown

@hujc7 hujc7 commented Apr 23, 2026

Description

Adds isaaclab.utils.checked_apply for forwarding the declared fields of one dataclass onto another, raising AttributeError if the target is missing a declared field.

The use case is Isaac Lab configclasses that mirror an upstream library's dataclass (for example, Newton's ShapeConfig). Bare setattr loops are fragile: if upstream renames or removes a field, every write becomes a silent no-op (the failure mode PR #5289 fixed for ShapeConfig.contact_marginmargin). With checked_apply, the failure surfaces at startup with a clear message instead of degrading runtime behavior.

API

from isaaclab.utils import checked_apply

@configclass
class NewtonShapeCfg:
    margin: float = 0.0
    gap: float = 0.01

# at apply site (one line, no per-field setattr noise)
checked_apply(cfg.default_shape_cfg, builder.default_shape_cfg)

Internally:

  1. Iterates dataclasses.fields(src) — single source of truth for declared fields.
  2. Raises AttributeError if target lacks a declared field.
  3. Rejects non-dataclass src with TypeError.

What's included

  1. source/isaaclab/isaaclab/utils/configclass.pychecked_apply function (lives next to @configclass since it operates on dataclasses).
  2. source/isaaclab/isaaclab/utils/__init__.pyi — export.
  3. source/isaaclab/test/utils/test_configclass.py — three tests (forwards all fields, raises on missing target field, rejects non-dataclass src).
  4. source/isaaclab/docs/CHANGELOG.rst4.6.13 entry.
  5. source/isaaclab/config/extension.toml — version bump.

Dependents

This PR is a dependency for the rough-terrain Newton stack:

  1. PR [Newton] Add Rough terrain locomotion Part 1: Foundation + quadrupeds on Newton #5248 — quadrupeds rough terrain, uses checked_apply to forward NewtonShapeCfg onto Newton's upstream ShapeConfig. Without it, default_shape_cfg.margin is left at Newton's upstream default of 0.0, breaking all non-Anymal-D robots on triangle-mesh terrain.
  2. PR [Rough Locomotion] Part 2: H1/Cassie bipeds on Newton #5298 — bipeds (chains on [Newton] Add Rough terrain locomotion Part 1: Foundation + quadrupeds on Newton #5248).
  3. PR [WIP][Rough Locomotion] Part 3: G1 on Newton (max_iterations 3000→5000, no physics tuning) #5312 — G1 (chains on [Rough Locomotion] Part 2: H1/Cassie bipeds on Newton #5298).

Type of change

  • New feature (non-breaking).

Checklist

  • Tests added (3 new in test_configclass.py)
  • Pre-commit checks pass
  • CHANGELOG + extension.toml bumped
  • No new dependencies

@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Apr 23, 2026
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Isaac Lab Review Bot

Summary

This PR introduces upstream_set, a defensive helper that guards against silent dead-attribute writes to upstream library configuration objects (dataclasses, modules). The motivation is sound—PR #5289 showed a real-world case where a Newton rename caused a stale assignment to silently become a no-op. The implementation is clean, well-tested, and correctly migrates the single existing cross-dep write site.

Architecture Impact

Self-contained. The new upstream_set utility has no dependencies beyond the standard library (dataclasses). The single migration point in NewtonManager.create_builder is low-risk—it affects builder initialization, not hot-path physics stepping. No other callers are impacted.

Implementation Verdict

Ship it — Minor documentation suggestion below, but nothing blocking.

Test Coverage

Thorough. The 5 unit tests cover:

  • ✅ Happy path: declared field is set correctly
  • ✅ Undeclared field raises AttributeError with helpful message containing field names
  • ✅ Failed write doesn't create spurious attribute
  • ✅ Module attribute that exists is set
  • ✅ Module attribute that doesn't exist raises

The tests use a synthetic _DummyCfg dataclass and types.ModuleType, which is appropriate—no need to depend on Newton's actual ShapeConfig in the isaaclab package tests.

CI Status

No CI results available yet. The PR description notes local pre-commit passed; awaiting CI run.

Findings

🔵 Improvement: source/isaaclab/isaaclab/utils/upstream.py:24 — Missing type annotation for value parameter

The value parameter lacks a type hint. While Any is semantically correct here (the function is type-agnostic), adding it improves IDE support and documentation consistency:

def upstream_set(obj: object, name: str, value: Any) -> None:

This requires from typing import Any (or from __future__ import annotations already present handles forward refs, but Any still needs import).

🔵 Improvement: source/isaaclab/isaaclab/utils/upstream.py:41-45 — Consider using type(obj) directly for module type path

For modules, type(obj).__module__ is builtins and type(obj).__name__ is module, producing builtins.module which is less helpful than the actual module's __name__. Consider:

if isinstance(obj, types.ModuleType):
    type_path = obj.__name__
else:
    type_path = f"{type(obj).__module__}.{type(obj).__name__}"

This would produce fake_upstream has no attribute 'quiet' instead of builtins.module has no attribute 'quiet'. Minor improvement for debugging module config writes.

🔵 Improvement: source/isaaclab/test/utils/test_upstream.py:52-55 — Test could verify error message content for module case

The module test verifies the exception is raised but doesn't assert on the message content. For consistency with test_undeclared_field_raises_attribute_error, consider:

def test_module_attribute_that_does_not_exist_raises():
    mod = types.ModuleType("fake_upstream")
    with pytest.raises(AttributeError) as excinfo:
        upstream_set(mod, "quiet", True)
    assert "quiet" in str(excinfo.value)

🔵 Improvement: source/isaaclab/isaaclab/utils/init.pyi:7-57 — Unrelated import reordering

The diff shows the .pyi stub was reformatted (alphabetized imports). This is fine—likely from running ruff format—but the diff is noisier than necessary. Not a problem, just noting it's unrelated to the feature.


Overall this is a well-executed, focused PR that addresses a real problem with an appropriately minimal solution. The convention documented in AGENTS.md ensures future cross-dep writes will use the helper.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 23, 2026

Greptile Summary

This PR adds checked_apply, a small utility that forwards every declared field of a source dataclass onto a target object, raising AttributeError if the target is missing any field. It replaces ad-hoc setattr loops that silently become no-ops when upstream APIs rename fields, and comes with three tests, a CHANGELOG entry, and a matching version bump.

Confidence Score: 5/5

Safe to merge — all findings are minor style suggestions with no impact on correctness or behavior.

The implementation is correct and well-tested. The two comments are P2 style preferences (consolidating the dual dataclasses import and using is_dataclass over a dunder check) that do not affect runtime behavior.

No files require special attention.

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/utils/configclass.py Adds checked_apply — a small, well-documented helper that iterates dataclasses.fields(src), validates each name against target, and forwards values via setattr. Logic is correct; only minor style concerns (redundant dual-import of dataclasses, dunder-attribute guard instead of is_dataclass).
source/isaaclab/isaaclab/utils/init.pyi Adds checked_apply to __all__ and to the lazy-loader import from .configclass. Consistent with how configclass and resolve_cfg_presets are already exposed.
source/isaaclab/test/utils/test_configclass.py Three focused tests covering the happy path (all fields forwarded, extra target fields untouched), missing-field error, and non-dataclass rejection. Good coverage of the specified contract.
source/isaaclab/docs/CHANGELOG.rst New 4.6.13 entry correctly documents the added helper with accurate date and Sphinx cross-reference.
source/isaaclab/config/extension.toml Patch version bumped from 4.6.12 to 4.6.13, consistent with the CHANGELOG entry.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["checked_apply(src, target)"] --> B{src has\n__dataclass_fields__?}
    B -- No --> C[raise TypeError]
    B -- Yes --> D["for f in dataclasses.fields(src)"]
    D --> E{hasattr\ntarget, f.name?}
    E -- No --> F[raise AttributeError\nwith target path + field name]
    E -- Yes --> G["setattr(target, f.name,\ngetattr(src, f.name))"]
    G --> D
    D -- done --> H[return None]
Loading

Reviews (2): Last reviewed commit: "Add checked_apply helper for forwarding ..." | Re-trigger Greptile

@hujc7 hujc7 force-pushed the jichuanh/upstream-set-helper branch from 351e5af to fcfb8ac Compare April 24, 2026 03:54
@hujc7 hujc7 requested a review from Mayankm96 as a code owner April 24, 2026 03:54
hujc7 added a commit to hujc7/IsaacLab that referenced this pull request Apr 24, 2026
Adds:
1. NewtonShapeCfg wrapper exposing per-shape collision defaults
   (margin, gap) via NewtonCfg.default_shape_cfg. NewtonManager.
   create_builder forwards it onto Newton's upstream
   ModelBuilder.default_shape_cfg via isaaclab.utils.checked_apply
   (PR isaac-sim#5365). Replaces the hardcoded gap=0.01 line with a single
   typed wrapper — the previous code never set margin, leaving it
   at Newton's upstream default of 0.0 and breaking all non-Anymal-D
   robots on triangle-mesh terrain.

2. Hoists Octi's per-env Anymal-D RoughPhysicsCfg into the shared
   LocomotionVelocityRoughEnvCfg so every rough-terrain env
   (Go1, Go2, A1, Anymal-B/C, H1, Cassie, Digit, G1) inherits
   identical Newton physics. The shared preset opts in to
   default_shape_cfg=NewtonShapeCfg(margin=0.01).

3. Per-env Newton-only tweaks where ablation showed measurable
   gains:
   - Go1: leg armature preset for joint stability.

4. Replaces additive (-5, 5) kg default on EventsCfg.add_base_mass
   with multiplicative (1/1.25, 1.25) log-uniform scale —
   scale-invariant across robot sizes, geometric mean 1.0, no
   per-robot kg overrides needed.

- isaaclab_newton 0.5.21 -> 0.5.22
- isaaclab_tasks  1.5.24 -> 1.5.25
@hujc7 hujc7 changed the title Add upstream_set helper for writes to upstream library configs Add checked_apply helper for forwarding configclass fields onto upstream dataclasses Apr 24, 2026
@hujc7 hujc7 force-pushed the jichuanh/upstream-set-helper branch from fcfb8ac to 698ab48 Compare April 24, 2026 05:59
@hujc7
Copy link
Copy Markdown
Author

hujc7 commented Apr 24, 2026

@greptileai Review — please re-review the latest commits.

@isaaclab-review-bot Please re-review.

Force-pushed with significant scope change since the previous review:

  1. Renamed upstream_setchecked_apply, changed API shape from single-field (obj, name, value) to bulk-apply (src_dataclass, target) driven by dataclasses.fields(src).
  2. Moved the helper from new file isaaclab/utils/upstream.py into existing isaaclab/utils/configclass.py (dataclass utilities belong together).
  3. Dropped the Newton create_builder migration from this PR — it's covered by dependent PR [Newton] Add Rough terrain locomotion Part 1: Foundation + quadrupeds on Newton #5248 where the consumer (NewtonShapeCfg) is also introduced.
  4. Dropped the AGENTS.md update (no longer needed).
  5. Tests moved from new test_upstream.py into existing test_configclass.py.

hujc7 added a commit to hujc7/IsaacLab that referenced this pull request Apr 24, 2026
Adds:
1. NewtonShapeCfg wrapper exposing per-shape collision defaults
   (margin, gap) via NewtonCfg.default_shape_cfg. NewtonManager.
   create_builder forwards it onto Newton's upstream
   ModelBuilder.default_shape_cfg via isaaclab.utils.checked_apply
   (PR isaac-sim#5365). Replaces the hardcoded gap=0.01 line with a single
   typed wrapper — the previous code never set margin, leaving it
   at Newton's upstream default of 0.0 and breaking all non-Anymal-D
   robots on triangle-mesh terrain.

2. Hoists Octi's per-env Anymal-D RoughPhysicsCfg into the shared
   LocomotionVelocityRoughEnvCfg so every rough-terrain env
   (Go1, Go2, A1, Anymal-B/C, H1, Cassie, Digit, G1) inherits
   identical Newton physics. The shared preset opts in to
   default_shape_cfg=NewtonShapeCfg(margin=0.01).

3. Per-env Newton-only tweaks where ablation showed measurable
   gains:
   - Go1: leg armature preset for joint stability.

4. Replaces additive (-5, 5) kg default on EventsCfg.add_base_mass
   with multiplicative (1/1.25, 1.25) log-uniform scale —
   scale-invariant across robot sizes, geometric mean 1.0, no
   per-robot kg overrides needed.

- isaaclab_newton 0.5.21 -> 0.5.22
- isaaclab_tasks  1.5.24 -> 1.5.25
…eam dataclasses

When an Isaac Lab configclass mirrors an upstream library's dataclass
(for example NewtonShapeCfg → Newton's ShapeConfig), bare setattr
loops are fragile: if upstream renames or removes a field, every
write becomes a silent no-op (the failure mode PR isaac-sim#5289 fixed for
ShapeConfig.contact_margin → margin).

Add isaaclab.utils.checked_apply(src, target):
  - Iterates fields(src) — single source of truth for declared fields
  - Raises AttributeError if target lacks a declared field — failure
    surfaces at startup, not at runtime
  - One-line apply at the call site, no per-field setattr noise
@hujc7 hujc7 force-pushed the jichuanh/upstream-set-helper branch from 698ab48 to f7202c0 Compare April 24, 2026 06:17
hujc7 added a commit to hujc7/IsaacLab that referenced this pull request Apr 24, 2026
Adds:
1. NewtonShapeCfg wrapper exposing per-shape collision defaults
   (margin, gap) via NewtonCfg.default_shape_cfg. NewtonManager.
   create_builder forwards it onto Newton's upstream
   ModelBuilder.default_shape_cfg via isaaclab.utils.checked_apply
   (PR isaac-sim#5365). Replaces the hardcoded gap=0.01 line with a single
   typed wrapper — the previous code never set margin, leaving it
   at Newton's upstream default of 0.0 and breaking all non-Anymal-D
   robots on triangle-mesh terrain.

2. Hoists Octi's per-env Anymal-D RoughPhysicsCfg into the shared
   LocomotionVelocityRoughEnvCfg so every rough-terrain env
   (Go1, Go2, A1, Anymal-B/C, H1, Cassie, Digit, G1) inherits
   identical Newton physics. The shared preset opts in to
   default_shape_cfg=NewtonShapeCfg(margin=0.01).

3. Per-env Newton-only tweaks where ablation showed measurable
   gains:
   - Go1: leg armature preset for joint stability.

4. Replaces additive (-5, 5) kg default on EventsCfg.add_base_mass
   with multiplicative (1/1.25, 1.25) log-uniform scale —
   scale-invariant across robot sizes, geometric mean 1.0, no
   per-robot kg overrides needed.

- isaaclab_newton 0.5.21 -> 0.5.22
- isaaclab_tasks  1.5.24 -> 1.5.25
hujc7 added a commit to hujc7/IsaacLab that referenced this pull request Apr 24, 2026
Adds:
1. NewtonShapeCfg wrapper exposing per-shape collision defaults
   (margin, gap) via NewtonCfg.default_shape_cfg. NewtonManager.
   create_builder forwards it onto Newton's upstream
   ModelBuilder.default_shape_cfg via isaaclab.utils.checked_apply
   (PR isaac-sim#5365). Replaces the hardcoded gap=0.01 line with a single
   typed wrapper — the previous code never set margin, leaving it
   at Newton's upstream default of 0.0 and breaking all non-Anymal-D
   robots on triangle-mesh terrain.

2. Hoists Octi's per-env Anymal-D RoughPhysicsCfg into the shared
   LocomotionVelocityRoughEnvCfg so every rough-terrain env
   (Go1, Go2, A1, Anymal-B/C, H1, Cassie, Digit, G1) inherits
   identical Newton physics. The shared preset opts in to
   default_shape_cfg=NewtonShapeCfg(margin=0.01).

3. Per-env Newton-only tweaks where ablation showed measurable
   gains:
   - Go1: leg armature preset for joint stability.

4. Replaces additive (-5, 5) kg default on EventsCfg.add_base_mass
   with multiplicative (1/1.25, 1.25) log-uniform scale —
   scale-invariant across robot sizes, geometric mean 1.0, no
   per-robot kg overrides needed.

- isaaclab_newton 0.5.21 -> 0.5.22
- isaaclab_tasks  1.5.24 -> 1.5.25
hujc7 added a commit to hujc7/IsaacLab that referenced this pull request Apr 24, 2026
Adds:
1. NewtonShapeCfg wrapper exposing per-shape collision defaults
   (margin, gap) via NewtonCfg.default_shape_cfg. NewtonManager.
   create_builder forwards it onto Newton's upstream
   ModelBuilder.default_shape_cfg via isaaclab.utils.checked_apply
   (PR isaac-sim#5365). Replaces the hardcoded gap=0.01 line with a single
   typed wrapper — the previous code never set margin, leaving it
   at Newton's upstream default of 0.0 and breaking all non-Anymal-D
   robots on triangle-mesh terrain.

2. Hoists Octi's per-env Anymal-D RoughPhysicsCfg into the shared
   LocomotionVelocityRoughEnvCfg so every rough-terrain env
   (Go1, Go2, A1, Anymal-B/C, H1, Cassie, Digit, G1) inherits
   identical Newton physics. The shared preset opts in to
   default_shape_cfg=NewtonShapeCfg(margin=0.01).

3. Per-env Newton-only tweaks where ablation showed measurable
   gains:
   - Go1: leg armature preset for joint stability.

4. Replaces additive (-5, 5) kg default on EventsCfg.add_base_mass
   with multiplicative (1/1.25, 1.25) log-uniform scale —
   scale-invariant across robot sizes, geometric mean 1.0, no
   per-robot kg overrides needed.

- isaaclab_newton 0.5.21 -> 0.5.22
- isaaclab_tasks  1.5.24 -> 1.5.25
Signed-off-by: Kelly Guo <kellyg@nvidia.com>
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Isaac Lab Review Bot

Summary

This PR adds a checked_apply helper function that forwards dataclass fields from an Isaac Lab configclass onto an external dataclass (e.g., Newton's ShapeConfig), raising AttributeError if the target is missing a declared field. The implementation is correct and the tests are well-structured, though there's one edge case worth noting.

Architecture Impact

Self-contained addition. The function lives in configclass.py alongside related utilities and is exported through __init__.pyi. No existing code is modified beyond the export. Downstream PRs (#5248, #5298, #5312) will use this to forward NewtonShapeCfg fields safely.

Implementation Verdict

Ship it — minor improvement suggested.

Test Coverage

Thorough for a utility of this scope:

  • test_checked_apply_forwards_all_fields: Validates happy path with mixed field values
  • test_checked_apply_raises_on_missing_target_field: Validates the core safety check
  • test_checked_apply_rejects_non_dataclass_src: Validates input type checking

All three tests use appropriate assertions and pytest.raises with match patterns.

CI Status

Core checks pass (pre-commit ✅, wheel build ✅, license ✅). Pending checks are infrastructure jobs that don't affect this utility code.

Findings

🔵 Improvement: source/isaaclab/isaaclab/utils/configclass.py:660-667 — Consider handling inherited fields explicitly

The function uses dataclasses.fields(src) which correctly includes inherited fields from parent dataclasses. This is the right behavior, but it's worth documenting since Isaac Lab configclasses often use inheritance chains. The current implementation handles this correctly; this is just a documentation note for the docstring.

🔵 Improvement: source/isaaclab/isaaclab/utils/configclass.py:657 — Error message could include the src type's module path for symmetry

The error message includes target_path with full module path but only type(src).__name__ without module. For consistency:

src_path = f"{type(src).__module__}.{type(src).__name__}"
raise AttributeError(
    f"{target_path} has no attribute `{f.name}`. {src_path} is out of sync with target."
)

🔵 Improvement: source/isaaclab/test/utils/test_configclass.py:1152 — Import could be at module level

The from isaaclab.utils import checked_apply import is repeated in each test function. While this works fine (and matches some other test patterns in this file), moving it to the top-level imports would be cleaner since it's not testing lazy import behavior.

🟡 Warning: source/isaaclab/isaaclab/utils/configclass.py:660 — MISSING sentinel values will be forwarded as-is

If src has a field set to MISSING (the dataclass sentinel), checked_apply will copy that sentinel to target. This is probably correct behavior (the caller should ensure src is fully populated), but worth being aware of since Isaac Lab configs commonly use MISSING. The dependent PRs should ensure configs are validated before calling checked_apply.


Overall this is a clean, focused addition that solves a real problem (silent no-ops when upstream APIs drift). The test coverage is appropriate and the implementation is correct.

Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Isaac Lab Review Bot

Summary

This PR adds a checked_apply utility function for safely forwarding dataclass fields from an Isaac Lab configclass to an external/upstream dataclass. The implementation is correct, well-documented, and properly tested. The function guards against the silent failure mode where upstream libraries rename fields (the bug class that PR #5289 fixed).

Architecture Impact

Self-contained. The new checked_apply function is a pure utility that:

  1. Lives in configclass.py alongside related dataclass utilities
  2. Is exported via __init__.pyi for public use
  3. Has no callers yet in this PR (dependents are listed as PRs #5248, #5298, #5312)

The function operates on standard Python dataclasses via dataclasses.fields(), making it compatible with both @configclass-decorated classes and plain @dataclass instances.

Implementation Verdict

Ship it — Minor suggestions below, but nothing blocking.

Test Coverage

Thorough for a utility of this scope:

  • test_checked_apply_forwards_all_fields — validates the happy path with mixed configclass src and plain dataclass target
  • test_checked_apply_raises_on_missing_target_field — validates the primary safety check
  • test_checked_apply_rejects_non_dataclass_src — validates input validation

The tests correctly use pytest.raises with match patterns. Edge cases covered include fields not declared on src being preserved on target.

CI Status

  • Check for Broken Links: failure — Unrelated to this PR (link checker failure in docs)
  • pre-commit: success — Formatting and linting pass
  • Build Wheel: success — Package builds correctly
  • ⏳ Several checks still pending (installation tests, docker builds)

Findings

🔵 Improvement: source/isaaclab/isaaclab/utils/configclass.py:661 — Consider adding target type validation

The function validates src is a dataclass but allows any object as target. While hasattr will work on any object, adding a note in the docstring that target can be any object with settable attributes (not necessarily a dataclass) would clarify intent, since the error message suggests structural compatibility:

# Current error message implies target should be a typed dataclass:
f"{target_path} has no attribute `{f.name}`. {type(src).__name__} is out of sync with target."

This is documentation-only; the behavior is correct.

🔵 Improvement: source/isaaclab/isaaclab/utils/configclass.py:666-668 — Error message could include src type path for symmetry

The error message includes the full path for target but only the class name for src. For consistency in debugging:

# Current:
f"{target_path} has no attribute `{f.name}`. {type(src).__name__} is out of sync with target."

# Suggested:
src_path = f"{type(src).__module__}.{type(src).__name__}"
f"{target_path} has no attribute `{f.name}`. {src_path} is out of sync with target."

This is a minor polish item.

🔵 Improvement: source/isaaclab/test/utils/test_configclass.py:1148-1210 — Test could verify field order preservation

The implementation iterates dataclasses.fields(src) which returns fields in declaration order. If order matters for any downstream use case (e.g., initialization order dependencies), a test verifying the iteration order would be valuable. However, since setattr is order-independent for simple attribute assignment, this is low priority.

🔵 Improvement: source/isaaclab/isaaclab/utils/init.pyi:59 — Export ordering

checked_apply is added at the end of __all__ (which is fine for functionality) but the import statement at line 110 places it before configclass. The __all__ list has it after resolve_cfg_presets. This inconsistency doesn't affect functionality but could be aligned for readability:

# Line 57: __all__ has order: configclass, resolve_cfg_presets, checked_apply
# Line 110: import has order: checked_apply, configclass, resolve_cfg_presets

Consider aligning these for consistency.


Overall: Clean, focused PR with correct implementation and good test coverage. The utility solves a real problem (silent no-ops on upstream field renames) with a simple, well-documented solution. Ready to merge once CI completes.

Signed-off-by: Kelly Guo <kellyg@nvidia.com>
Signed-off-by: Kelly Guo <kellyg@nvidia.com>
@kellyguo11 kellyguo11 merged commit d76c153 into isaac-sim:develop Apr 26, 2026
32 checks passed
kellyguo11 added a commit that referenced this pull request Apr 26, 2026
… on Newton (#5248)

# Description

Enables Newton rough-terrain locomotion training on all locomotion
velocity envs (Go1, Go2, A1, Anymal-B/C/D, H1, Cassie, Digit, G1) on top
of [@ooctipus](https://github.com/ooctipus)'s Anymal-D foundation work,
cherry-picked from PR #5225.

## Why this PR exists

PR #5225 (Octi's draft) added Newton support for Anymal-D rough terrain.
The other 9 locomotion envs were left out of scope. Octi is away and his
PR has CI failures, so per maintainer guidance ([Kelly Guo's
comment](#5225 (comment)))
the required changes from #5225 are cherry-picked here so the
rough-terrain stack can move forward without depending on his still-open
WIP PR.

## Dependencies

- PR #5365 — adds `isaaclab.utils.checked_apply`, used by
`NewtonManager.create_builder` to forward `NewtonShapeCfg` onto Newton's
upstream `ShapeConfig`. Required for stable rough-terrain contact.

## 1. Cherry-pick from #5225 (`614ea2dbb74`)

Commit authored by Octi Zhang with `Co-authored-by` trailer. Subset of
#5225 — what's kept and dropped:

| # | Item | Status | Reason |
|---:|---|---|---|
| 1 | `anymal_d/rough_env_cfg.py` Anymal-D Newton config | **KEPT**
(then hoisted into shared parent in commit 2 below) | Defines the Newton
physics shape used for rough terrain |
| 2 | `velocity_env_cfg.py` — hoist `physics_material`, `add_base_mass`,
`base_com` startup events into shared `EventsCfg` | **KEPT** | All envs
need them |
| 3 | `base_com` guard `preset(default=..., newton=None)` | **KEPT** |
Ablation A4 on #5225 (posted 2026-04-17): without the gate, 99.99% of
episodes terminate from `body_lin_vel` runaway on Newton. Upstream
Newton fix newton-physics/newton#2332 will let us drop the guard once it
ships |
| 4 | `velocity_env_cfg.py` — `collider_offsets` startup event |
**DROPPED** | (a) Greptile P1 on #5225: PhysX-only `root_view` methods,
would `AttributeError` on Newton without a guard. (b) Ablation A3:
clobbers the 1cm shape margin set by `RoughPhysicsCfg` (event resets
`gap = max(0, contact_offset − margin) = 0`). Removing it gives
**+3.71** reward on Anymal-D Newton (+16.38 vs A0 baseline +12.47) |
| 5 | `terminations.py` — `body_lin_vel_out_of_limit` /
`body_ang_vel_out_of_limit` + `__init__.pyi` exports | **DROPPED** |
Were a NaN guard for the Newton `body_lin_vel` runaway when `base_com`
was unguarded. With the `preset(newton=None)` gate (item 3), the runaway
no longer occurs and the guards are unused |
| 6 | `terrain_generator.py` subdivided flat-grid border | **DROPPED** |
Was a workaround for Newton triangle-collision failures on the
box-primitive border. Newton has since improved triangle handling, so
the workaround is no longer needed |

## 2. New work — `2a532d1f745`

### 2.1 `NewtonShapeCfg` + `checked_apply` wiring

The single most important Newton setting for rough terrain is **shape
margin**. Without a nonzero margin, non-Anymal-D robots fail to learn
stable contact on triangle-mesh terrain. The previous
`NewtonManager.create_builder` only set `gap = 0.01` and left `margin`
at Newton's upstream default of `0.0`.

This PR adds `NewtonShapeCfg` (the Isaac Lab wrapper) exposing `margin`
and `gap`, and forwards it onto Newton's upstream `ShapeConfig` via
`checked_apply` from PR #5365:

```python
@configclass
class NewtonShapeCfg:
    margin: float = 0.0
    gap: float = 0.01

# in NewtonCfg
default_shape_cfg: NewtonShapeCfg = NewtonShapeCfg()

# in NewtonManager.create_builder
shape_cfg = cfg.default_shape_cfg if isinstance(cfg, NewtonCfg) else NewtonShapeCfg()
checked_apply(shape_cfg, builder.default_shape_cfg)
```

`RoughPhysicsCfg` opts in to
`default_shape_cfg=NewtonShapeCfg(margin=0.01)`.

### 2.2 Hoist `RoughPhysicsCfg` into shared base

Octi's per-env Anymal-D `RoughPhysicsCfg` (MJWarp solver + collision
pipeline) is hoisted into `LocomotionVelocityRoughEnvCfg.sim` so every
rough-terrain env inherits identical Newton physics. Per-env files
become minimal robot-only deltas.

### 2.3 Per-env Newton-only tweak

- **Go1**: leg armature preset for joint stability on lightweight
quadruped.

### 2.4 Mass randomization rewrite

Replace `EventsCfg.add_base_mass`'s additive `(-5, 5)` kg default with
multiplicative `(1/1.25, 1.25)` log-uniform scale (`operation="scale"`,
`distribution="log_uniform"`). Scale-invariant across robot sizes,
geometric mean 1.0, no per-robot kg overrides needed.

Per-robot ablation, Newton, 1500-iter (small quadrupeds early-aborted at
iter 300):

| # | Robot | new log-uniform | old additive baseline | ratio | verdict
|
|---:|---|---:|---:|---|---|
| 1 | A1 (iter 300) | 10.00 | 3.25 | **3.08×** | pass — driven by bias
removal (old `(-1, 3)` had +10% mean bias on A1's 10 kg base) |
| 2 | Go1 (iter 300) | 22.29 | 16.30 | 1.37× | pass |
| 3 | Anymal-B (iter 300) | 12.47 | 10.92 | 1.14× | pass |
| 4 | Anymal-C (iter 300) | 14.64 | 12.31 | 1.19× | pass |
| 5 | Go2 @ 1499 | 24.71 | 18.58 | 1.33× | pass |
| 6 | Anymal-D @ 1499 | 16.09 | 15.62 | 1.03× | pass |
| 7 | H1 @ 1499 (biped) | 24.02 | 23.58 | 1.02× | pass |
| 8 | Cassie @ 1499 sym25 | 14.15 | 23.93 (mass rand off) | 0.59× |
**regression — fixed in dependent PR #5298 with `(1.0, 1.25)`
heavier-only override** |

Cassie sensitivity is closed-loop Achilles + hip PD response:
lighter-than-nominal pelvis destabilizes; heavier-only `(1.0, 1.25)`
recovers 90% of the reward and pushes ep_len higher (932 vs 910
baseline). Sub-ablation table is in #5298.

Raw logs, checkpoints, and config snapshots preserved at
`~/workspaces/data/2026-04-21_mass-rand-scale/` (per-robot
`<robot>_newton_1500.log`, `key.md`, `status.md`, `run.sh` reproducer).

## Versions

- `isaaclab_newton` 0.5.21 → 0.5.22
- `isaaclab_tasks` 1.5.24 → 1.5.25

## Type of change

- New feature (non-breaking).

## Checklist

- [x] Pre-commit checks pass
- [x] CHANGELOG + extension.toml bumped on both `isaaclab_newton` and
`isaaclab_tasks`
- [x] Co-author credit for [@ooctipus](https://github.com/ooctipus) on
the cherry-pick commit
- [x] Ablation evidence cited in commit messages

---------

Signed-off-by: Kelly Guo <kellyg@nvidia.com>
Co-authored-by: Octi Zhang <zhengyuz@nvidia.com>
Co-authored-by: Kelly Guo <kellyg@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants